-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update memory calculator #5244
base: dev
Are you sure you want to change the base?
update memory calculator #5244
Conversation
83ab2f0
to
702b222
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks mostly good to me, though I made some mostly just pedantic comments.
row_division = np.full(global_gpu_extent[dim], int(global_cell_extent[dim] / global_gpu_extent[dim]), dtype=np.int_) | ||
global_domain_division.append(row_division) | ||
|
||
print(f"global domain division: {global_domain_division}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, it is not clear what domain division is supposed to be. I think you should name it differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after an offline discussion I followed Pawel's advice and renamed it to grid_distribution
to match more closely the PIConGPU terminology.
|
||
print(f"global domain division: {global_domain_division}") | ||
|
||
# get cell extent of each GPU, <extent of the gpu domain>[simulation dimension, multi dimensional gpu index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get this notation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrased the comment to make it clearer.
raise ValueError("unsupported precision {precision}") | ||
|
||
@staticmethod | ||
def get_predefined_attribute_dict(simulation_dimension: int, precision: int) -> dict[str, int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a static method? simulation_dimension
, precision
are also instance attributes. So as long as you put in the class, I would make just use these values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since these methods might be usable in other contexts I decided to make them independent of the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BrianMarre ok, but in that case I would just take it out of the class :)
ci: no-compile
ci: no-compile
ci: no-compile
702b222
to
8593bdb
Compare
ci: no-compile
8593bdb
to
79ee6ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have strong opinions about the qualities of my bike's accommodation but there's actually one thing about the counting in bits and bytes that needs reworking. Others are optional. Haven't checked if the details of the memory usage actually match up with what the code does.
self.value_size * num_fields * local_cells | ||
+ double_buffer_mem | ||
+ self.value_size * num_pml_fields * local_pml_cells | ||
number_local_cells = int(np.prod(cell_extent + self.super_cell_size * 2 * self.guard_size)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not quite obvious what cell_extent
refers to. It looks like it means extent_in_number_of_cells
while I first read it as extent_of_a_cell
. Better name would be great.
Forgot: Offline discussion in the meeting: We want to keep the main example minimal and concrete (and not have the "kind of general" computation for varying parameters) and might add the "how to compute the used values" in a separate section. |
done |
This PR,